Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: improve shellcheck fix messages #284

Merged
merged 15 commits into from
Jan 17, 2025
Merged

feat: improve shellcheck fix messages #284

merged 15 commits into from
Jan 17, 2025

Conversation

thatRichman
Copy link
Contributor

@thatRichman thatRichman commented Jan 10, 2025 •
edited
Loading

Describe the problem or feature in addition to a link to the issues.

Partially addresses #276 .

This PR improves ShellCheck lint messages by parsing, when available, the fix field and applying the suggested replacements. To enable this, the fix module was added which is closely modeled after shellcheck's own Fixer. The intent for this module is for it be usable by other lint rules to suggest and apply their own replacements using a consistent framework, but this is out-of-scope for this PR.

Before submitting this PR, please make sure:

  • You have added a few sentences describing the PR here.
  • You have added yourself or the appropriate individual as the assignee.
  • You have added at least one relevant code reviewer to the PR.
  • Your code builds clean without any errors or warnings.
  • You have added tests (when appropriate).
  • You have updated the README or other documentation to account for these
    changes (when appropriate).
  • You have added an entry to the relevant CHANGELOG.md (see
    "keep a changelog" for more information).
  • Your commit messages follow the [conventional commit] style.

@thatRichman thatRichman force-pushed the feat/improve-fix-messages branch from eda5bdd to 08024e5 Compare January 10, 2025 00:10
@thatRichman
Copy link
Contributor Author

thatRichman commented Jan 10, 2025 •
edited
Loading

There's opportunity here to take some of the warnings that shellcheck does not currently emit fixes for and create our own, but I think this may not be a good idea for a couple of reasons.

  1. It means that if shellcheck is updated to begin emitting fixes, we're overriding that.
  2. Much of the low-hanging fruit is actually fairly complicated because shellcheck will emit multiple warnings for the same line, (e.g. [[foo]] emits three conflicting suggestions), of which we would have to decide priority.

@a-frantz
Copy link
Member

There's opportunity here to take some of the warnings that shellcheck does not currently emit fixes for and create our own, but I think this may not be a good idea for a couple of reasons.

  1. It means that if shellcheck is updated to begin emitting fixes, we're overriding that.
  2. Much of the low-hanging fruit is actually fairly complicated because shellcheck will emit multiple warnings for the same line, (e.g. [[foo]] emits three conflicting suggestions), of which we would have to decide priority.

Agree that we shouldn't pursue adding our own fixes to ShellCheck errors. If ShellCheck doesn't supply a fix, neither should we.

a-frantz
a-frantz reviewed Jan 12, 2025
View reviewed changes
Copy link
Member

@a-frantz a-frantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just nits related to variable naming and getting doc comments to link to definitions.

wdl-lint/Cargo.toml Outdated Show resolved Hide resolved
wdl-lint/src/fix.rs Show resolved Hide resolved
wdl-lint/src/fix.rs Outdated Show resolved Hide resolved
wdl-lint/src/fix.rs Outdated Show resolved Hide resolved
wdl-lint/src/fix.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/shellcheck.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/shellcheck.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/shellcheck.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/shellcheck.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/shellcheck.rs Outdated Show resolved Hide resolved
peterhuene
peterhuene reviewed Jan 16, 2025
View reviewed changes
Copy link
Collaborator

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good and something we might build upon for a general purpose fixing framework for lint diagnostics in the future.

Just a few nits and minor comments.

wdl-lint/src/fix.rs Outdated Show resolved Hide resolved
wdl-lint/src/fix.rs Show resolved Hide resolved
wdl-lint/src/fix.rs Outdated Show resolved Hide resolved
wdl-lint/src/fix.rs Show resolved Hide resolved
a-frantz
a-frantz approved these changes Jan 16, 2025
View reviewed changes
Copy link
Member

@a-frantz a-frantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after Peter's comments are addressed

peterhuene
peterhuene approved these changes Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good (pending fix to the CI).

Thanks again for this work as it'll definitely improve the UX with shellcheck installed!

@a-frantz a-frantz merged commit bbe761a into stjude-rust-labs:main Jan 17, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-frantz a-frantz a-frantz approved these changes

@peterhuene peterhuene peterhuene approved these changes

@adthrasher adthrasher Awaiting requested review from adthrasher

@claymcleod claymcleod Awaiting requested review from claymcleod

Assignees

@thatRichman thatRichman

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thatRichman @a-frantz @peterhuene